Stop the lint pipeline from reporting issues it re-creates itself - #5788
Stop the lint pipeline from reporting issues it re-creates itself#5788jurgenwerk wants to merge 1 commit into
Conversation
The realm lint pipeline runs prettier before template-lint. Prettier wraps text nodes longer than its print width across indented lines, and the no-whitespace-for-layout rule flags exactly that wrap with no autofix — so for any over-width text node the pipeline reported an error no edit could clear, and the AI assistant looped forever re-patching whitespace that the formatter reverted on every save. The template linter now runs with that rule disabled; the wrapped whitespace collapses at render, so nothing is lost by not flagging it. As a generic backstop, PatchCodeTool now detects when the lint/format pass reverts an applied patch back to the original file content. It skips the no-op save and reports alongside the lint issues that reformatting-only patches cannot make progress, so the model stops retrying. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb1dfa4bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...hostTemplateLintConfig, | ||
| rules: { | ||
| ...hostTemplateLintConfig.rules, | ||
| 'no-whitespace-for-layout': false, |
There was a problem hiding this comment.
Preserve whitespace linting for standalone HBS files
This disables the rule for every template-lint invocation, including .hbs submissions. In lintOne, .hbs is absent from ESLINT_EXTENSIONS, so it never passes through the Prettier step that motivates this exception, but it is included in TEMPLATE_LINT_EXTENSIONS; consequently genuine whitespace-for-layout violations in standalone templates are now silently accepted. Apply the override only for .gts/.gjs inputs that were formatted, or retain a separately configured linter for .hbs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] The routing half of this is correct and I reproduced it: .hbs is in TEMPLATE_LINT_EXTENSIONS but absent from ESLINT_EXTENSIONS, so lintOne never runs prettier over it, and driving lintSource over <div>\n <span>A B</span>\n</div> reports no-whitespace-for-layout on main and reports nothing on this branch.
The remedy doesn't follow, though, because the premise it rests on — that prettier's wrapping is what motivates the exception — isn't true for .gts either. no-whitespace-for-layout trims each line of a text node before matching, so prettier's newline-plus-indent is invisible to it; on main, a long plain-space text node that prettier wraps reports nothing, while a short unwrapped line containing reports the rule. The trigger is adjacency in the authored text, independent of formatting. Details and the full fixture table are in my review comment on initTemplateLinter in packages/runtime-common/tasks/lint.ts.
So the rule was never conditioned on the prettier pass, and gating the override on "inputs that were formatted" would split behavior along an axis the rule doesn't respond to: two files with identical text would lint differently based only on extension. Once the justification is restated in the terms that actually hold — runs are deliberate visual spacing in realm content, the rule has no autofix, and its message doesn't identify the offending characters — disabling it uniformly across .gts/.gjs/.hbs is the consistent choice rather than an oversight. My read is that no code change is needed here, only the comment rewrite I've asked for separately.
Generated by Claude Code
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] I reviewed this as a claim-verification pass. The change rests on one specific mechanical story — that prettier's wrapping and no-whitespace-for-layout contradict each other by construction — and that story is checkable, so I checked it: I drove lintSource from packages/runtime-common/tasks/lint.ts directly (real host config, real ESLint → prettier → ESLint → template-lint chain, ember-template-lint 7.9.3), on main and on this branch, over a set of fixtures that separate wrapping from runs.
Bottom line: the code does the right thing and the config swap is safe — I verified rule-for-rule that nothing else in the template-lint chain changed. But the mechanism the change is documented by does not hold, and both the code comment and the new test encode it. No functional blockers; I'd hold merge only on the comment, because it's the sole explanation the next editor gets and it points them at the wrong part of the pipeline.
What lands right
The config-object swap is the risky part of this change and it comes out clean. Passing config inline could easily have dropped the extends chain or broken the relative plugins: ['../template-lint/plugin'] path; it doesn't. Old and new linters agree on every fixture I tried except the one rule being disabled — plugin rules (no-data-test-selector, require-scoped-style, no-unused-block-params-except-underscore), core recommended rules (no-triple-curlies, no-forbidden-elements, no-curly-component-invocation), and the plugin's own opt-outs (require-button-type staying off) all behave identically. Existing {{! template-lint-disable no-whitespace-for-layout }} comments in realm content don't turn into unused-directive errors either.
Scoping the save into the else branch is also right: skipping determineFinalFileUrl and trackAiAssistantCardRequest alongside the write means a no-op patch doesn't burn a filename probe, a tracked request id, or a loader reset.
The mechanism, corrected
no-whitespace-for-layout trims each line of a text node before matching, so prettier's newline-plus-indent is invisible to it. On main: a long plain-space text node that prettier wraps reports nothing; a short unwrapped <span>A B</span> reports the rule. The trigger is adjacency in the authored text, independent of formatting. And the error is clearable — ...HOMEPAGE HOT SITE... lints clean and survives prettier. What doesn't survive is a whitespace-only edit like collapsing the wrap, which prettier restores byte-for-byte. So the loop you reproduced is real; its cause is that Excess whitespace detected, anchored on the whole text node, never says which whitespace, so the model kept choosing the one edit class the formatter undoes. Full evidence table in the inline comment on initTemplateLinter.
That doesn't argue against the disable — runs are deliberate spacing in card content, the rule has no autofix, and its diagnostic is unactionable. It argues for saying that in the comment.
Answering the open thread
The Codex thread about .hbs is mechanically correct — I reproduced it: .hbs is in TEMPLATE_LINT_EXTENSIONS but not ESLINT_EXTENSIONS, so it never meets prettier, and on main an .hbs fixture reports the rule while on this branch it doesn't. But its remedy is premised on the prettier rationale, which doesn't hold. Once the justification is restated as " runs are legitimate in realm content", disabling for .hbs is the consistent choice, not a gap. I've replied in that thread; my read is no code change is needed there.
Recommendations
- Rewrite the
initTemplateLintercomment to the rationale that survives checking. Suggested wording in the inline thread onpackages/runtime-common/tasks/lint.ts. (the one I'd block on) - Rename the new lint test and add the short-line fixture that pins the disable's actual scope — see the thread on
lint-test.ts. Worth adding a fixture that trips a different template-lint rule while you're there; theextendschain surviving the swap is this change's real risk and nothing covers it. - Tighten
revertedByFormatterto require that the pre-lint content actually differed, and cover the new branch inpatch-code-test.gts— see the thread onpatch-code.ts.
Adjacent, out of scope
- The backstop is advisory, not mechanical. The result still reports
appliedwith the extra issue,checkCorrectnessconcatenates every lint issue intoerrorsregardless of severity, socorrect: falsestands and another patch round is still offered.MAX_CORRECTNESS_FIX_ATTEMPTSdoesn't bound this either:formatCorrectnessTargetKeyWithEventkeys the attempt counter ontargetEventId, so each new patch event resets it to 1 — which is exactly why the observed loop was unbounded despite a 3-attempt cap existing. The added sentence is a prompt-level nudge and worth having; the PR description's "so the model stops retrying instead of looping" is stronger than what's enforced. - Severity is flattened on the correctness path.
formatLintIssuesreturns errors and warnings alike, andcheckCorrectnessconcatenates all of them intoerrors. So demoting a noisy rule towarninstead offalsewould not have helped here — worth knowing before someone reaches for that as the gentler option next time.
Generated by Claude Code
| // Host's config, minus rules the rest of this pipeline contradicts. | ||
| // Prettier (the pass right before template-lint) wraps text nodes longer | ||
| // than its print width across indented lines; no-whitespace-for-layout | ||
| // flags exactly that wrap and has no autofix, so for any over-width text | ||
| // node the pipeline would report an error that no edit can clear — the | ||
| // formatter reverts every attempted whitespace fix. The wrapped whitespace | ||
| // collapses at render, so nothing is lost by not flagging it. |
There was a problem hiding this comment.
[Claude Code 🤖] This comment states a mechanism that doesn't hold: prettier's wrapping never triggers no-whitespace-for-layout. The disable itself may well be right, but this explanation is the only thing the next editor will read, and it will send them to fix the wrong thing (reorder the pipeline, re-enable the rule "now that prettier runs later", etc.).
What the rule actually does. ember-template-lint/lib/rules/no-whitespace-for-layout.js visits each TextNode, splits its source on \n, trims every line, and only then tests /(( )|( ))(( )|( ))/:
let lines = source.split('\n');
for (let line of lines) {
// ignore whitespace at the start and end of the line
let trimmed = line.trim();
// check for two ` ` or ` ` in a row
let matches = trimmed.match(/(( )|( ))(( )|( ))/g);Prettier's wrap contributes exactly a newline plus leading indentation — both removed by that trim. What the regex matches is two adjacent “space or ” tokens inside a trimmed line, which is a property of the authored text, not of the formatting.
Verified against the real pipeline. I drove lintSource from this file (so the full ESLint → prettier → ESLint → template-lint chain, host config, ember-template-lint 7.9.3) on main, i.e. with the rule still enabled:
| fixture (text node contents) | prettier wrapped it? | message |
|---|---|---|
long text, plain single spaces, no |
yes | (none) |
short line A B |
no | no-whitespace-for-layout |
long line with runs |
yes | no-whitespace-for-layout |
A B (two literal spaces) |
no | (none) — prettier rewrote it to A B |
Row 1 is the direct disproof of "prettier wraps → the rule flags the wrap": the wrap happened and nothing was reported. Row 2 shows the rule firing with no wrapping anywhere in play. Row 4 shows the one kind of whitespace prettier does touch, and it resolves the rule rather than fighting it.
"An error that no edit can clear" is also not the case. Rewriting the run so no sits next to a space — ...MY HOMEPAGE HOT SITE AWARD WINNER BEST VIEWED... — lints clean, and prettier still wraps that line, so the edit survives the formatter. What does not survive is a whitespace-only edit such as collapsing the wrapped text back onto one line: prettier restores it byte-identically. So the loop you reproduced is real, but its cause is that the rule's message — Excess whitespace detected, anchored on the whole text node — never says which whitespace it means, and the model kept picking the one edit class the formatter undoes.
The way out. Keep the disable, and justify it on the ground that actually holds: runs are deliberate visual spacing in card content, the rule has no autofix, and its diagnostic doesn't identify the offending characters, so it produces errors the patch loop can't act on. Something like:
// Host's config, minus rules that are noise for realm content.
// no-whitespace-for-layout flags any two adjacent " " / " " tokens in a
// trimmed line of a text node. Card content uses runs deliberately for
// visual spacing, the rule has no autofix, and its message ("Excess whitespace
// detected", anchored on the whole text node) doesn't identify which
// whitespace it means — so it hands the patch loop an error it can't act on.Scope: regression introduced by this PR (the comment is new), non-blocking for behavior. I'd still hold merge on it — a wrong mechanism in a comment costs more than the missing comment would.
Confirmation, separately: the config-object swap itself is safe. I instantiated new TemplateLinter({ workingDir: HOST_PKG }) and the new { workingDir, config } form side by side and compared them on eight fixtures. The full extends chain survives: the @cardstack/template-lint plugin rules still load and fire (no-data-test-selector, require-scoped-style, no-unused-block-params-except-underscore), core recommended rules still fire (no-triple-curlies, no-forbidden-elements, no-curly-component-invocation), and rules the plugin config turns off stay off (require-button-type). hostRequire('./.template-lintrc.js') resolves against the host package directory as intended, and an existing {{! template-lint-disable no-whitespace-for-layout }} in realm content does not become an unused-directive error. The one standing condition: this hard-codes the config filename, so moving host to .template-lintrc.cjs or a package.json key would break it silently rather than at a config-resolution error.
Generated by Claude Code
| test('does not flag the whitespace prettier introduces by wrapping long text', async function (assert) { | ||
| // Prettier wraps text nodes longer than its print width across | ||
| // indented lines; no-whitespace-for-layout flags exactly that wrap and | ||
| // has no autofix, so reporting it would hand back an issue no edit can | ||
| // clear — the formatter reverts every attempted whitespace fix. | ||
| let longText = | ||
| 'WELCOME TO MY HOMEPAGE HOT SITE AWARD WINNER BEST VIEWED IN 800x600 '; |
There was a problem hiding this comment.
[Claude Code 🤖] This test passes for a different reason than its name and comment claim, so it pins a narrower contract than the change actually makes.
The test does fail on main and pass here, so it guards something real. But the wrap it asserts on line 652 isn't why no-whitespace-for-layout fired: as detailed in the comment on initTemplateLinter in packages/runtime-common/tasks/lint.ts, the rule trims each line before matching, so prettier's wrap is invisible to it — the runs already present in longText are what fire it. I confirmed on main that a long text node with plain spaces and no gets wrapped by prettier and reports nothing, while <span>A B</span> on a single short line reports no-whitespace-for-layout without any wrapping.
Two consequences:
- The name mis-teaches.
does not flag the whitespace prettier introduces by wrapping long textdescribes a behavior that was never there to remove. Someone later reading this test will carry the same wrong model of the pipeline that thelint.tscomment encodes. - The real contract is broader than what's pinned. The change disables the rule for every template-lint invocation, not just wrapped-text cases. Nothing in the suite would notice if a future edit narrowed the disable to some formatter-introduced subset, because this fixture would keep passing either way.
The way out. Rename to what it checks — something like no-whitespace-for-layout is not reported for realm content — drop the wrap assertion (or keep it, but stop labelling it as the cause), and add a second fixture with the run on a short, prettier-stable line. That second case is the one that pins the actual scope: it fails on main for exactly the same reason with no wrapping involved.
While you're in here, a .gts fixture that still trips a different template-lint rule would be worth adding — the extends chain surviving the config-object swap is the main risk in this change and nothing in the suite covers it. <template><div>{{{this.html}}}</div></template> (no-triple-curlies, core recommended) and <template><style>.a { color: red }</style></template> (require-scoped-style, the @cardstack/template-lint plugin) both fire through this pipeline; I verified both against the new linter. A plugin-rule case in particular would catch a future config regression that silently drops the plugin.
Scope: follow-up-sized, but it's in this PR's own new test, so it's cheap to do here. Non-blocking.
Generated by Claude Code
| // An applied patch always changed the pre-lint content, so ending up | ||
| // back at the original file means the autofix/format pass reverted | ||
| // the whole edit. Re-patching can never make progress from here — | ||
| // any further attempt round-trips to this same content — so say so | ||
| // alongside the lint issues instead of writing a no-op save that | ||
| // would keep the model trying. | ||
| let revertedByFormatter = | ||
| sourceContent !== '' && patchedCode === sourceContent; | ||
| if (revertedByFormatter) { | ||
| lintIssues = [ | ||
| ...lintIssues, | ||
| 'The automatic formatter reverted the applied changes, so the file is unchanged. Reformatting-only patches cannot fix the remaining issues; change the content itself or leave it as is.', | ||
| ]; |
There was a problem hiding this comment.
[Claude Code 🤖] The flag infers "the formatter reverted it" from "the content is unchanged", and those come apart in two reachable cases. Both are minor; the fix is two lines and makes the code state the invariant instead of inheriting it.
Why the premise holds today. ApplySearchReplaceBlockTool.run throws SEARCH_PATTERN_NOT_FOUND when resultContent === input.fileContent && searchPattern !== '', so a single block that changes nothing lands as failed, not applied. That's what makes "an applied patch always changed the pre-lint content" true — but it's true by a rule enforced in a different file, with nothing at either end noting the dependency. If that guard is ever relaxed (a debatable one: it also rejects a legitimate patch whose only effect prettier would have made anyway), this branch starts lying without any test noticing.
Where it comes apart now.
- Non-lintable targets.
isLintableFilematches only/\.(gts|ts)$/, so.jsoncard instances — a common patch target — never reachlintAndFix. Same for any.gtswhose patched content is blank (patchedCode.trim() !== ''gate above). In those paths no formatter ran at all, yet a content-equal outcome would tell the model "The automatic formatter reverted the applied changes". - Blocks that cancel. Two blocks in one
codeBlocksarray where the second undoes the first each change content individually, so both reportappliedand neither throws, butpatchedCode === sourceContent.
Neither is common, and in both the skip-the-save half is the right call. It's the message that misleads — and it's aimed at a model that will act on it.
The way out. Capture the pre-lint content and require that it differed, so the flag means what its name says:
let preLintCode = patchedCode;
if (patchedCode.trim() !== '' && this.isLintableFile(fileUrl)) {
let lintResult = await this.lintAndFix(fileUrl, patchedCode);
patchedCode = lintResult.output;
lintIssues = lintResult.lintIssues ?? [];
}
let revertedByFormatter =
sourceContent !== '' &&
preLintCode !== sourceContent &&
patchedCode === sourceContent;Test coverage. The new branch has none. packages/host/tests/integration/tools/patch-code-test.gts already installs an adapter.lintStub, so a stub that returns the request body's pre-patch content gives you the case directly: assert the file on the realm is untouched, that lintIssues carries the new sentence, and — the part that actually matters for the loop — that results[0].status is still applied. That last assertion is worth pinning explicitly, because gatherPatchedFiles in packages/runtime-common/ai/prompt.ts skips any result whose status isn't applied; if this branch ever started reporting failed, the message would be assembled into the prompt and then dropped, and the backstop would silently stop working.
Scope: regression-adjacent (the misleading message is new), non-blocking. The test gap is the part I'd most want addressed before merge.
Generated by Claude Code
The AI assistant could loop forever on a correctness check it could never satisfy. The realm lint pipeline runs prettier before template-lint, and the two disagree by construction for any text node longer than prettier's 80-column print width:
Every "fix" patch applied, was rewrapped to byte-identical content, and produced the same two lint messages — reproduced end to end against a real looping session's file.
Two changes:
no-whitespace-for-layoutdisabled. Prettier's wrapping is canonical in this pipeline and the whitespace collapses at render, so the rule only ever contradicted the formatter. (Host source already silences this rule inline where it conflicts.)PatchCodeTooldetects when the lint/format pass reverts an applied patch back to the original content: it skips the no-op save and adds a lint issue saying reformatting-only patches cannot make progress, so the model stops retrying instead of looping.A lint-endpoint test covers the wrapped-long-text case; other template-lint rules from the
extendschain still fire (verified against the pipeline directly).